Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PHP 8 string offset warnings in getColumnName and addIndex #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saracubillas
Copy link

  • Updated getColumnName method to handle cases where an empty array is passed, setting the fieldName to null to avoid PHP 8 string offset warnings.
  • Modified addIndex method to ensure 'fields' are always treated as an array, preventing potential issues with non-array types.
  • Added checks in both methods to ensure compatibility with PHP 8 while maintaining backward compatibility with older PHP versions.

These changes resolve the PHP 8 warnings related to string offsets in the Table class of the Doctrine integration within our Symfony project.

- Updated getColumnName method to handle cases where an empty array is passed, setting the fieldName to null to avoid PHP 8 string offset warnings.
- Modified addIndex method to ensure 'fields' are always treated as an array, preventing potential issues with non-array types.
- Added checks in both methods to ensure compatibility with PHP 8 while maintaining backward compatibility with older PHP versions.

These changes resolve the PHP 8 warnings related to string offsets in the Table class of the Doctrine integration within our Symfony project.
@alquerci
Copy link

Hello @saracubillas,

Welcome and thank for your contribution.

It looks indeed fix a wrong value type.

I am curious about the case where the issue you want to solve appears.

Some questions.

  • Does it comes from YAML schema configuration?
  • Does it comes from some default settings provided by symfony1?
  • Does it comes from a symfony1 plugin?

To be able to reproduce it.

Can you share an example of Doctrine integration within Symfony project ?

@saracubillas
Copy link
Author

saracubillas commented Apr 11, 2024

hi @alquerci !

  • Does it come from YAML schema configuration?

Yes, the issue is related to the YAML schema configuration. The task sfDoctrineBuildModelTask reads the schema that configuration to generate the model classes. The way single-column and multi-column indexes are defined in schema.yml is directly influencing how the corresponding PHP code is generated.

  • Does it come from some default settings provided by Symfony1?

It's not directly a "default setting" of Symfony1, the behavior is a result of how the Symfony1 Doctrine plugin interprets and processes the YAML schema configurations.
Does it come from a Symfony1 plugin?

  • Yes, this behavior is part of the sfDoctrinePlugin in Symfony1. The sfDoctrineBuildModelTask class, which is a part of this plugin, is responsible for reading the schema.yml file and generating the model classes accordingly.

Example of Doctrine Integration within a Symfony Project:

# schema.yml
Product:
  columns:
    id:
      type: integer
      primary: true
      autoincrement: true
    name:
      type: string(255)
    price:
      type: decimal
  indexes:
    product_name_index:
      fields: name  # Single field index
# schema.yml
Order:
  columns:
    id:
      type: integer
      primary: true
      autoincrement: true
    product_id:
      type: integer
    customer_id:
      type: integer
    quantity:
      type: integer
  indexes:
    order_product_customer_index:
      fields: [product_id, customer_id]  # Multi-field index

execute the
php symfony doctrine:build-model
look for the setTableDefinition() on the base models generated:

$this->index(' product_name_index', array(
'fields' => 'name',
));

$this->index('order_product_customer_index', array(
'fields' =>
array(
0 => 'product_id',
1 => 'customerr_id',
),
));

@alquerci
Copy link

alquerci commented Apr 12, 2024

# schema.yml
Product:
  columns:
    id:
      type: integer
      primary: true
      autoincrement: true
    product_id:
      type: integer
    customer_id:
      type: integer
  indexes:
    # change break this
    single_field_index:
      fields: product_id

    multi_field_index:
      fields:
        - product_id
        - customer_id
    
    # the change make this kind of index without PHP warning
    empty_composite_foreign_key_index:
       fields:
         - []

    composite_foreign_key_index:
       fields:
         - ['customer_id']

What kind of index configuration match the issue?
If no one match, can you provide them?

@saracubillas
Copy link
Author

single_field_index:
fields: product_id
the above index is provoking the warnings, I have tried to fix with my pull. Is this your question?

@alquerci
Copy link

single_field_index: fields: product_id the above index is provoking the warnings, I have tried to fix with my pull. Is this your question?

Yes, you answered to my question.

@alquerci
Copy link

I added test for your use case. They failed, without surprise.

See #133

Do you have this following PHP warning?

PHP Warning: Only the first byte will be assigned to the string offset in /home/runner/work/doctrine1/doctrine1/lib/Doctrine/Table.php on line 885
PHP Fatal error: Uncaught TypeError: Doctrine_Export_Sqlite::getIndexFieldDeclarationList(): Argument #1 ($fields) must be of type array, string given, called in /home/runner/work/doctrine1/doctrine1/lib/Doctrine/Export/Sqlite.php on line 122 and defined in /home/runner/work/doctrine1/doctrine1/lib/Doctrine/Export/Sqlite.php:134

source

@saracubillas
Copy link
Author

the first warning was my problem. The Fatal I did not have it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants